[TASK] Add Tests for Document::getAllDeclarationBlocks()
#956
Conversation
oliverklee
added
the
testing
oliverklee
added this to the
9.0.0: Drop support for PHP < 7.2 and clean up things milestone
oliverklee
force-pushed
the
tests/get-all-declaration-blocks
branch
from
1a44e9b to
a1accfb
Compare
JakeQZ
left a comment
JakeQZ
left a comment
There was a problem hiding this comment.
The tests look fine. But I find them difficult to follow because I can't immediately see how $this->subject has been setUp(). Would it perhaps be clearer for the subject to be set up on a per-test basis? And maybe a separate pre-PR to remove the setUp() method, which I don't think is warranted.
oliverklee
force-pushed
the
tests/get-all-declaration-blocks
branch
from
a1accfb to
245d68a
Compare
|
Done: #963 |
oliverklee
force-pushed
the
tests/get-all-declaration-blocks
branch
from
245d68a to
e822151
Compare
|
Updated an repushed. |
JakeQZ
left a comment
JakeQZ
left a comment
There was a problem hiding this comment.
I think a test method name should not include 'also', and that it would be better to provide non-empty strings which are ideally real-world examples for URL and charset. Otherwise fine.
oliverklee
force-pushed
the
tests/get-all-declaration-blocks
branch
from
9651087 to
7fac016
Compare
JakeQZ
left a comment
JakeQZ
left a comment
There was a problem hiding this comment.
The method name issue does not seem to be resolved.
Also remove a misleading and incorrect comment. Part of #757
oliverklee
force-pushed
the
tests/get-all-declaration-blocks
branch
from
7fac016 to
997eaf7
Compare
Indeed. My bad. Done. |
Also remove a misleading and incorrect comment.
Part of #757